[Form Refactor] ACHContractStep #13501
Conversation
| firstName: 'firstName', | ||
| lastName: 'lastName', | ||
| dob: 'dob', | ||
| ssnLast4: 'ssnLast4', | ||
| street: 'beneficialOwnerAddressStreet', | ||
| city: 'beneficialOwnerAddressCity', | ||
| state: 'beneficialOwnerAddressState', | ||
| zipCode: 'beneficialOwnerAddressZipCode', |
There was a problem hiding this comment.
These keys should be dynamic
There was a problem hiding this comment.
I'll look into a solution for these since we might need to make changes to the Form component itself
There was a problem hiding this comment.
@grgia I took a look at dynamically adding/removing IdentityForm and the solution below should work:
- Store
beneficialOwnersin local state like we do now. However, we'll only store an ID for theIdentityForminstead of the full form input keys. No need to change anything here, we'll do so in the other methods that use state. - Update
addBeneficialOwnerto store this ID inbeneficialOwnersand set the Form draft values accordingly. Something like:
addBeneficialOwner() {
this.setState((prevState) => {
const beneficialOwners = [...prevState.beneficialOwners, NumberUtils.rand64()];
// We set 'beneficialOwners' to null first because we don't have a way yet to replace a specific property without merging it.
// We don't use the debounced function because we want to make both function calls.
FormActions.setDraftValues(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {beneficialOwners: null});
FormActions.setDraftValues(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {beneficialOwners});
return {beneficialOwners};
});
}
- Update
removeBeneficialOwnerto take the ID of theIdentityFormwe are removing and update the Form draft accordingly. Like so:
removeBeneficialOwner(beneficialOwner) {
this.setState((prevState) => {
const beneficialOwners = _.without(prevState.beneficialOwners, beneficialOwner);
// We set 'beneficialOwners' to null first because we don't have a way yet to replace a specific property without merging it.
// We don't use the debounced function because we want to make both function calls.
FormActions.setDraftValues(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {beneficialOwners: null});
FormActions.setDraftValues(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {beneficialOwners});
return {beneficialOwners};
});
}
- In the render method, map over the
beneficialOwnersstate, which now stores IDs, and pass a key in the formatbeneficialOwner.${id}.firstNameto bothdefaultValuesandinputKeys. Something like:
defaultValues={{
firstName: ReimbursementAccountUtils.getDefaultStateForField(this.props, `beneficialOwner.${id}.firstName`),
...
}}
inputKeys={{
firstName: `beneficialOwner.${id}.firstName`,
...
}}
- Add
onValueChange={value => this.setState({hasOtherBeneficialOwners: value})}to thehasOtherBeneficialOwnerscheckbox. - Update the call to
this.removeBeneficialOwnerto pass the ID of theIdentityFormwe are removing. - Update the
validatefunction to use the new keys.
_.each(this.state.beneficialOwners, (id) => {
if (!ValidationUtils.isRequiredFulfilled(values[`beneficialOwner.${id}.firstName`])) {
errors[`beneficialOwner.${id}.firstName`] = this.props.translate('bankAccount.error.firstName');
}
...
- The main issue with this approach is that it doesn't update the state in Form, so it could still pass data stored for deleted
IdentityForms(if the user removed a beneficial owner as they are editing the form). The only drawback that I can think of is that we will have to filter these values in thesubmitfunction so we don't send them with the API request. I think this is a fine trade off considering that it makes the rest of the implementation easier.
Let me know what you think of this approach!
There was a problem hiding this comment.
This method looks like it's working well! I wrote out my plan for testing, so I still need to QA edge cases
There was a problem hiding this comment.
Sounds good! I'll take another look today! Thanks for working on this.
There was a problem hiding this comment.
OK I am still a little confused, but haven't yet given this a proper investigation to produce any alternatives. Something does not sit right with me about the random id thing. I feel like I would have remembered this from the Design Doc (maybe it didn't come up). It has the feeling of something we might want to bring up in Slack, decide together the best way to do it, and then update everyone on the process (maybe modify the very awesome FORMS.md doc 😉).
There was a problem hiding this comment.
I don't think this came up in the doc (at least I don't recall it either). IMO the random id solution solves this issue quite well, but I'm open to discussing this further in Slack if others prefer that.
There was a problem hiding this comment.
I'd love to see this get to the finish line this week, but I'm happy to hold this for a discussion and move forward with whatever is decided (final testing for the current method or implementing a new method). I agree that the current method works well, and also that it would be worth updating everyone on the process and why it's done this way in FORMS.md
There was a problem hiding this comment.
so, sounds like we agree that it's a good thing to discuss and that you are offering to lead a discussion on it? Or no? 😄
It might feel like a small battle - but I am concerned with this solution because it feels like something I would never remember how to do or know the reason why a random ID is used. I am barely understanding the explanation about why it is needed. I'd say that's a sign we haven't solved it in the best way possible (maybe I'm wrong). But if it has to be difficult to understand then at least we can document it better.
|
Implemented the first pass of suggestions! |
There was a problem hiding this comment.
@grgia PR is looking good! Just a few small comments/suggestions to address and we should be good!
|
@grgia let me know once this is ready for another round of review! |
|
Just finishing up testing the latest branch! I'll ping you once I'm done |
|
Okay, @luacmartins I tested again and it's working well! Ready for another review or move this out of draft :) |
|
Fixed the lint errors! |
src/pages/ReimbursementAccount/ReimbursementAccountDraftPropTypes.js
Outdated
Show resolved
Hide resolved
Reviewer Checklist
Screenshots/VideosWeb13501.Web.-.QA.Identity.Form.Validation.mov13501.Web.-.QA.Required.Validation.mov13501.Web.-.QA.Beneficial.Owners.-.Identity.Form.Flow.mov13501.Web.-.QA.Form.Submissions.movMobile Web - Chrome13501.mWeb-Chrome.-.QA.Identity.Form.Validation.mov13501.mWeb-Chrome.-.QA.Required.Validation.mov13501.mWeb-Chrome.-.QA.Beneficial.Owners.-.Identity.Form.Flow.mov13501.mWeb-Chrome.-.QA.Form.Submissions.movMobile Web - Safari13501.mWeb-Safari.-.QA.Beneficial.Owners.-.Identity.Form.Flow.mp413501.mWeb-Safari.-.QA.Required.Validation.mp413501.mWeb-Safari.-.QA.Form.Submissions.mp413501.mWeb-Safari.-.QA.Identity.Form.Validation.mp4Desktop13501.Desktop.-.QA.Identity.Form.Validation.mov13501.Desktop.-.QA.Beneficial.Owners.-.Identity.Form.Flow.mov13501.Desktop.-.QA.Required.Validation.mov13501.Desktop.-.QA.Form.Submissions.moviOS13501.iOS.-.QA.Required.Validation.mp413501.iOS.-.QA.Beneficial.Owners.-.Identity.Form.Flow.mp413501.iOS.-.QA.Identity.Form.Validation.mp413501.iOS.-.QA.Form.Submissions.movAndroid13501.Android.-.QA.Required.Validation.mov13501.Android.-.QA.Identity.Form.Validation.mov13501.Android.-.QA.Beneficial.Owners.-.Identity.Form.Flow.mov13501.Android.-.QA.Form.Submissions.mov |
|
Just waiting for the checklist here and then we are should be good to merge! |
|
Merging since @marcaaron had already approved these changes! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.2.64-0 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀
|
| @@ -95,7 +99,7 @@ class Form extends React.Component { | |||
|
|
|||
| this.state = { | |||
| errors: {}, | |||
| inputValues: {}, | |||
| inputValues: props.draftValues, | |||
There was a problem hiding this comment.
This change in the PR caused #14784 regression.
Title: Settings - Changed name is not saved under Display name page
Steps to reprodcue:
- Click on >Setting>Profile > Display name
- Change name>Click Save
- Click the back arrow
- Click the Display name again
- The new name does not match the old name
|
I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check. We've also updated the item in the checklist with this PR to avoid this issue in the future. |
Details
Form Refactor for ACH Contract Step of Add Bank Account Form.
Fixed Issues
$ #9578
Tests
Offline tests
QA Steps
QA Required Validation:
Click submit without checking any boxes
Click submit checking only "I accept the terms and conditions"
Click submit checking only " I certify that ..."
Check both boxes, ensure no error messages shown
QA Beneficial Owners / Identity Form Flow:
Check "Someone else owns more than 25%..." > Ensure that IdentityForm is created
Check "Someone else owns more than 25%..." > Click "Add another individual who owns more than 25% of Alberta Bobbeth Charleson" > Continue clicking that until you cannot add more forms. > Ensure that only 4 forms are created
Check "Someone else owns more than 25%..." AND "I own more than 25%" > Click "Add another individual who owns more than 25% of Alberta Bobbeth Charleson" > Continue clicking that until you cannot add more forms. > Ensure that only 3 forms are created
QA Identity Form Validation
Check both required checks AND "Someone else owns more than 25%..." > Fill out 1 Identify Form correctly > then 1 by 1 remove or incorrectly fill out each field (first name, last name, dob, ssn, address, city, state, zipcode) > Ensure form does not submit
Check both required checks AND "Someone else owns more than 25%..." > Fill out DOB such that the age is <18, and ensure an error is shown
QA Form Submissions
Check both required boxes > click submit > ensure bank account is successfully connected with no beneficial owners
Check both required checks AND "Someone else owns more than 25%..." > Fill out 1 Identify Form correctly > Ensure form submits with no error and data is correct.
Check both required checks AND "Someone else owns more than 25%..." > Fill out 1 Identify Form correctly then delete it > add another identity form and fill it out correctly > Ensure form submits with no error and data is correct.
Check both required checks AND "Someone else owns more than 25%..." > add 3 identity forms and fill 2 out correctly and 1 incorrectly > Remove the incorrect form > Ensure form submits with no error and data is correct.
Check all checkmarks > Fill out multiple Identify Forms correctly > Ensure form submits with no error and data is correct.
Check all four boxes > Fill out 1 Identify Form correctly > Ensure form submits with no error and data is correct.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Screen.Recording.2022-12-16.at.2.20.21.PM.mov
Mobile Web - Chrome
Mobile Web - Safari
Screen.Recording.2022-12-16.at.2.24.29.PM.mov
Desktop
Screen.Recording.2022-12-16.at.2.53.45.PM.mov
iOS
Screen.Recording.2022-12-16.at.2.25.41.PM.mov
Android
Screen.Recording.2022-12-16.at.2.27.01.PM.mov